fix: use federated graph id and org id to fetch operation content#2107
Conversation
JivusAyrus
requested review from
Aenimus,
StarpTech,
jensneuse and
wilsonrivera
as code owners
WalkthroughThis change introduces two new fields, Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Router image scan passed✅ No security vulnerabilities found in image: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
proto/wg/cosmo/platform/v1/platform.proto (1)
582-586: Consider declaring the new fields asoptionaland add doc-comments for consistency
GetOperationContentRequestnow relies onfederated_graph_nameandnamespace, but they are declared as plainstringfields.
In proto3 this means:
- No presence tracking – the backend cannot distinguish between “not provided” and “empty string”.
- Call-sites must always transmit a (possibly empty) string, which easily leads to subtly broken filters.
Unless the service really needs empty-string semantics, make them explicitly
optionaland add short comments (as done on most neighbouring messages) to keep the schema self-documenting.- string federated_graph_name = 2; - string namespace = 3; + // FQDN of the federated graph whose operation content should be fetched + optional string federated_graph_name = 2; + // Namespace the graph belongs to + optional string namespace = 3;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
connect-go/gen/proto/wg/cosmo/platform/v1/platform.pb.gois excluded by!**/*.pb.go,!**/gen/**
📒 Files selected for processing (7)
connect/src/wg/cosmo/platform/v1/platform_pb.ts(2 hunks)controlplane/src/core/bufservices/analytics/getOperationContent.ts(2 hunks)controlplane/src/core/repositories/CacheWarmerRepository.ts(2 hunks)proto/wg/cosmo/platform/v1/platform.proto(1 hunks)studio/src/components/checks/operation-content.tsx(3 hunks)studio/src/components/checks/operations.tsx(1 hunks)studio/src/components/checks/override.tsx(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
studio/src/components/checks/operations.tsx (1)
studio/src/components/checks/operation-content.tsx (1)
OperationContentDialog(86-126)
controlplane/src/core/bufservices/analytics/getOperationContent.ts (1)
controlplane/src/core/repositories/FederatedGraphRepository.ts (1)
FederatedGraphRepository(96-1831)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: build-router
- GitHub Check: integration_test (./. ./fuzzquery ./lifecycle ./modules)
- GitHub Check: image_scan (nonroot)
- GitHub Check: build_push_image
- GitHub Check: build_push_image (nonroot)
- GitHub Check: integration_test (./telemetry)
- GitHub Check: integration_test (./events)
- GitHub Check: image_scan
- GitHub Check: build_test
- GitHub Check: build_test
- GitHub Check: build_push_image
- GitHub Check: build_push_image
- GitHub Check: build_test
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
connect/src/wg/cosmo/platform/v1/platform_pb.ts (2)
4514-4522: LGTM! Property definitions follow protobuf conventions.The new string properties are correctly defined with appropriate TypeScript types, empty string defaults, and proper @generated JSDoc comments.
4533-4534: LGTM! Field descriptors are correctly structured.The field descriptors properly define the new fields with sequential field numbers (2, 3), correct scalar string type, and appropriate snake_case naming convention for protobuf.
controlplane/src/core/repositories/CacheWarmerRepository.ts (1)
99-100: LGTM! Proper scoping implementation.The addition of
OrganizationIDandFederatedGraphIDfiltering in the SQL query correctly implements the intended scoping to prevent cross-organization and cross-graph data access. The parameter integration is clean and follows existing patterns.Also applies to: 119-121
studio/src/components/checks/operation-content.tsx (2)
24-25: LGTM! Clean prop propagation with proper validation.The addition of
federatedGraphNameandnamespaceprops correctly propagates the federated graph context. The conditional query enabling (enabled: enabled && !!federatedGraphName && !!namespace) is a good defensive practice that ensures the API call only happens when all required parameters are available.Also applies to: 29-30, 38-42
89-90: LGTM! Consistent prop forwarding in dialog component.The
OperationContentDialogcomponent properly accepts and forwards the new props to theOperationContentcomponent, maintaining consistency in the component hierarchy.Also applies to: 94-95, 117-122
controlplane/src/core/bufservices/analytics/getOperationContent.ts (3)
10-10: LGTM! Proper import for federated graph validation.The import of
FederatedGraphRepositoryenables proper validation of federated graph access permissions.
34-44: LGTM! Excellent security validation.The federated graph validation ensures users can only access graphs within their organization scope. The error handling with descriptive messages (
Federated graph '${req.federatedGraphName}' not found) provides good user feedback while maintaining security.
46-53: LGTM! Proper query scoping with performance optimization.The SQL query correctly filters by both
OrganizationIDandFederatedGraphIDto prevent data leakage. The addition of query cache settings (use_query_cache = true, query_cache_ttl = 2629800) is a good performance optimization for this type of lookup query.
…while-fetching-the-operation
… in operation content
…while-fetching-the-operation
…while-fetching-the-operation
…while-fetching-the-operation
JivusAyrus
deleted the
suvij/eng-6195-use-the-fedgraphid-and-orgid-while-fetching-the-operation
branch
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Checklist